Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Setup SQLite tests #477

Merged
merged 21 commits into from
Sep 11, 2024
Merged

Setup SQLite tests #477

merged 21 commits into from
Sep 11, 2024

Conversation

martosaur
Copy link
Contributor

@martosaur martosaur commented Jul 6, 2024 •
edited by woylie
Loading

Context: #446 (comment)

This PR changes test setup so that it could run against multiple databases. I copied the ecto_sql approach here. A quick summary:

  1. Tests are not split into test and integration_test folders. The former can run on their own, the latter require a db.
  2. mix test only runs tests in test folder
  3. mix test.all runs mix test with ECTO_ADAPTER={adapter name} for each adapter defined in @adapters attribute in mix.exs. Depending on the env variable, mix test runs tests in integration_test/{adapter name}. Each folder contains test_helper.exs which runs adapter-specific setup (start repo and run migrations). Each folder also contains all_test.exs file which imports adapter-agnostic test cases from integration_test/cases/. All adapter-specific tests can be put directly in adapter folder, i.e. integration_test/pg/prefix_test.exs
  4. I left postgres migrations in priv/migrations so that they could be run in dev environment.
  5. SQLite has its own version of migrations as it does not support creating schemas or custom types.

My goal here was to make tests possible, so I didn't look into fixing failing ones.

I hope this helps!

  • produce complete coverage report (HTML and JSON)
  • refactor GH actions workflow (separate jobs per adapter, sqlite as optional check for now)
  • skip prefix/schema tests for sqlite

@martosaur martosaur requested a review from woylie as a code owner July 6, 2024 02:59
@martosaur martosaur changed the title Am/sqlite tests Setup SQLite tests Jul 6, 2024
@woylie
Copy link
Owner

woylie commented Jul 14, 2024 •
edited
Loading

Thank you for this! I merged main into your branch, which splits the coverage report from the test matrix for the Elixir and OTP versions.

I wonder whether it makes sense to run tests for all adapters in the version matrix or whether it would make more sense to only use the Postgres adapter in the version matrix, and test the other adapters on the latest Elixir/OTP version only. It would also be nice to have separate CI jobs for each adapter, so that you can see where it goes wrong more easily.

There are different approaches we could take:

  • test-adapters: adapter matrix, latest Elixir/OTP version only, coverage report stored as artifact and imported
  • test-versions: Elixir/OTP version matrix with Postgres adapter only, no coverage
  • code quality: latest Elixir/OTP only, as before

Or:

  • test-adapters: postgres and sqlite in separate steps, latest Elixir/OTP version only, coverage report only generated and reported for postgres adapter (but then I can't make the sqlite adapter tests optional until all issues have been fixed)
  • test-versions: Elixir/OTP version matrix with Postgres adapter only, no coverage
  • code quality: latest Elixir/OTP only, as before

Or:

  • test-postgres: latest Elixir/OTP version only, with coverage
  • test-sqlite: latest Elixir/OTP version only, without coverage
  • test-versions: Elixir/OTP version matrix with Postgres adapter only, no coverage
  • code quality: latest Elixir/OTP only, as before

For ergonomics, it would also be nice to be able to just type mix test postgres or mix test.postgres or mix test --adapter postgres.

@martosaur
Copy link
Contributor Author

Thanks for looking into this! I don't have much experience with GH actions, so ultimately your call. Personally, I like the last option with explicit test-postgres and test-sqlite jobs.

For ergonomics, it would also be nice to be able to just type mix test postgres or mix test.postgres or mix test --adapter postgres.

I added both test.postgres and test.sqlite aliases.

Let me know if there is anything pending from my side for this PR!

@woylie
Copy link
Owner

woylie commented Aug 7, 2024

Thanks, I'll do the rest (don't know when yet, but it's on my list).

@woylie woylie merged commit 6ae8884 into woylie:main Sep 11, 2024
15 checks passed
@codecov Codecov
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.67%. Comparing base (d2e2a58) to head (c2aed56).
Report is 31 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #477   +/-   ##
=======================================
  Coverage   87.67%   87.67%           
=======================================
  Files          15       15           
  Lines         933      933           
=======================================
  Hits          818      818           
  Misses        115      115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@woylie
Copy link
Owner

woylie commented Sep 11, 2024

I pushed a couple of changes and disabled the sqlite tests on the CI until they are fixed. Thanks again!

@martosaur martosaur deleted the am/sqlite_tests branch September 11, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woylie woylie Awaiting requested review from woylie woylie is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martosaur @woylie